fix(datagrid): Delete key and right-click respect multi-row/cell-range selection#1648
fix(datagrid): Delete key and right-click respect multi-row/cell-range selection#1648J2TeamNNL wants to merge 1 commit into
Conversation
…e selection
D12A: delete(_:) checks gridSelection.affectedRows first, mirroring copy().
D12B: rightMouseDown override skips super when clicking inside existing selection,
calls NSMenu.popUpContextMenu directly to preserve the selection set.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f37596613
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let controller = gridSelection, !controller.isEmpty { | ||
| let rows = controller.selection.affectedRows | ||
| guard !rows.isEmpty else { return } | ||
| coordinator?.delegate?.dataGridDeleteRows(Set(rows)) |
There was a problem hiding this comment.
When the selected range includes newly inserted rows, dataGridDeleteRows(Set(rows)) physically removes those rows, but the grid selection is left intact: applyRemovedRows only updates/removes table rows and syncSelection performs the next row selection programmatically, so tableViewSelectionDidChange does not clear selectionController. After deleting a cell range over inserted rows, the Delete menu remains enabled from the stale grid selection and pressing Delete again deletes whatever rows shifted into the old indexes.
Useful? React with 👍 / 👎.
| override func rightMouseDown(with event: NSEvent) { | ||
| let point = convert(event.locationInWindow, from: nil) | ||
| let clickedRow = row(at: point) | ||
| if clickedRow >= 0, selectedRowIndexes.contains(clickedRow) { |
There was a problem hiding this comment.
This fast path only recognizes rows in selectedRowIndexes, but a grid cell-range selection can span many rows while the NSTableView row selection contains only the focused row. Right-clicking a selected cell on another row falls through to super.rightMouseDown, which selects the clicked row; tableViewSelectionDidChange then clears selectionController when the change is not programmatic, so the context menu still appears after collapsing the cell-range selection. The condition needs to also treat clicks inside gridSelection.selection as selected.
Useful? React with 👍 / 👎.
1 participant
Root cause
D12A —
delete(_:)only readselectedRowIndexes. When cells are selected via drag (grid-range selection),selectedRowIndexescan be empty, so Delete did nothing.D12B — NSTableView's default
rightMouseDownreplacesselectedRowIndexeswith just the clicked row beforemenu(for:)is called. By the time the context menu appeared, the multi-row selection was gone.Fix
D12A —
delete(_:)now checksgridSelection.affectedRowsfirst, then falls back toselectedRowIndexes. Mirrors the existingcopy()pattern exactly.validateUserInterfaceItemanddeleteSelectedRowsIfPossibleupdated to match.D12B —
rightMouseDownoverride: if the clicked row is already inside the current selection, skipsuper(which would collapse it) and callNSMenu.popUpContextMenu(_:with:for:)directly.File:
TablePro/Views/Results/KeyHandlingTableView.swiftTest plan